Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Get rid of fc/io/fstream.hpp #2047

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

pmconrad
Copy link
Contributor

@pmconrad pmconrad commented Nov 7, 2019

Part of #1116 / #1584

@abitmore abitmore added this to the 4.1.0 - Feature Release milestone Nov 8, 2019
#include <string>

namespace graphene { namespace utilities {

/**
* @brief Reads the file at the given path and returns the full contents in a string. Note that the result
* will contain non-printable characters if the file does.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I saw "non-printable characters", '\0' didn't come to my mind. I don't know whether it's only me though. Perhaps better if we explicitly mention '\0'? Callers need to be careful when using this function, since functions like c_str() are heavily used in the code base, a strcpy() or strlen() call on the result of c_str() may return unexpected results.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor fc::fstream to be more like std::fstream
2 participants